Skip to content

stop tailscale service after CI workload finishes #206

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2025
Merged

stop tailscale service after CI workload finishes #206

merged 1 commit into from
Oct 20, 2025

Conversation

@oxtoacart
Copy link
Collaborator

@oxtoacart oxtoacart commented Oct 17, 2025

Updates #205

@oxtoacart oxtoacart requested a review from jaxxstorm October 17, 2025 13:43
@oxtoacart oxtoacart force-pushed the percy/issue205 branch 2 times, most recently from 9090315 to 7f5fd03 Compare October 17, 2025 13:46
@oxtoacart oxtoacart marked this pull request as ready for review October 17, 2025 13:46
@oxtoacart oxtoacart removed the request for review from jaxxstorm October 17, 2025 13:50
@oxtoacart oxtoacart force-pushed the percy/issue205 branch 3 times, most recently from 835c63c to bd56b76 Compare October 17, 2025 13:58
oxtoacart
oxtoacart commented Oct 17, 2025
View reviewed changes
src/logout/logout.ts Outdated
}
// The pid is actually the pid of the `sudo` parent of tailscaled, so use pkill -P to kill children of that parent
await exec.exec("sudo", ["pkill", "-P", pid]);
core.info("✅ Stopped tailscaled");
Copy link
Collaborator Author

@oxtoacart oxtoacart Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, I don't have a way of running something after this in my test workflow to verify it worked.

To test manually, I temporarily logged the output of ps -ef both before and after stopping tailscaled, and you can see the results of that for Ubuntu and MacOS.

@oxtoacart oxtoacart requested a review from jaxxstorm October 17, 2025 14:00
@jaxxstorm
Copy link
Contributor

jaxxstorm commented Oct 18, 2025

@oxtoacart I approved this, but I thought of a better way.

tailscaled has a -cleanup flag which should handle stopping everything

-cleanup
    	clean up system state and exit

We could potential net stop on Windows as well, maybe that's a better approach?

@oxtoacart
Copy link
Collaborator Author

oxtoacart commented Oct 20, 2025

tailscaled has a -cleanup flag which should handle stopping everything

I tried this on my Mac, and it doesn't stop any running tailscaled processes. Also, the cleanup that it does do happens on every run of tailscaled.

I do think it would be good to run after we kill tailscaled in order to leave DNS and routes in a good state.

We could potential net stop on Windows as well

It would be nice to support Windows, let me try that.

@oxtoacart oxtoacart changed the title on Linux and MacOS, stop tailscaled after CI workload finishes stop tailscale service after CI workload finishes Oct 20, 2025
@jaxxstorm
Copy link
Contributor

jaxxstorm commented Oct 20, 2025

huh, that's sad. I do wonder if service tailscaled stop would be sufficient for Linux, but then we're relying on systemd to do its job properly

@oxtoacart
Copy link
Collaborator Author

oxtoacart commented Oct 20, 2025

I do wonder if service tailscaled stop would be sufficient for Linux

We're not installing tailscale as a service, we're just spawning the tailscaled daemon manually.

@oxtoacart
Copy link
Collaborator Author

oxtoacart commented Oct 20, 2025

Here's a CI run with temporary logging code that shows us successfully stopping the tailscale daemon and tailscale-ipn.exe on Windows.

@oxtoacart oxtoacart requested a review from jaxxstorm October 20, 2025 16:20
@oxtoacart oxtoacart merged commit 2ddbe90 into main Oct 20, 2025
14 checks passed
@oxtoacart oxtoacart deleted the percy/issue205 branch October 20, 2025 17:53
@oxtoacart oxtoacart mentioned this pull request Oct 20, 2025
Closed
@oxtoacart oxtoacart mentioned this pull request Oct 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jaxxstorm jaxxstorm jaxxstorm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@oxtoacart @jaxxstorm